Update to termtest v2 #2731

Naatan · 2023-08-23T20:59:53Z

No description provided.

# Conflicts: # cmd/state-installer/test/integration/installer_int_test.go # go.sum # test/integration/auth_int_test.go # test/integration/bundle_int_test.go # test/integration/package_int_test.go # test/integration/push_int_test.go # test/integration/revert_int_test.go # vendor/modules.txt

Fix tests to be termtest compatible

…lementation did, despite the name

# Conflicts: # go.sum # test/integration/install_scripts_int_test.go # test/integration/update_int_test.go

# Conflicts: # internal/testhelpers/e2e/session.go

Naatan · 2023-09-20T18:59:29Z

@mitchell-as I suggest giving the e2e changes a closer look, but to only review the rest at a very high level.

mitchell-as

This was a herculean effort that you managed to pull off. No amount of applause is enough.
In addition to the overhaul, I like that you also added reasonable timeouts or removed unnecessary ones.

Nevertheless, there are a few things that we should probably address.

.github/workflows/build.yml

mitchell-as · 2023-09-20T20:14:35Z

cmd/state-svc/test/integration/svc_int_test.go

 	cp.Expect("Starting")
 	time.Sleep(1 * time.Second) // wait for the service to start up
-	cp.Signal(syscall.SIGINT)
+	cp.Cmd().Process.Signal(syscall.SIGINT)


This is not very discoverable. Other uses of cp.Cmd() are in session.go, which is fine for lower-level stuff, but high level stuff like sending process signal should be a top-level function in my opinion. Are you open to changing this back to cp.Signal(syscall.SIGINT)?

Edit: is this any different than cp.SendCtrlC()? Something to think about.

Originally I had this implemented at a higher level, but it quickly becomes burdensome to handle these states when it comes to closing the connections when it's done. In the end I felt that making the command a responsibility of the consumer was better as it's rare that you even need to do this, and this way we're not trying to do too much with the termtest library.

Making it accessible via a Cmd() method is itself already going slightly further than required as the command is fed into termtest by the consumer, so the consumer should already have access to this regardless.

Okay, so this is different than cp.SendCtrlC()?

internal/testhelpers/e2e/session.go

mitchell-as · 2023-09-20T20:19:19Z

internal/testhelpers/e2e/session.go

@@ -187,6 +178,7 @@ func new(t *testing.T, retainDirs, updatePath bool, extraEnv ...string) *Session
 		constants.OptinUnstableEnvVarName + "=true",
 		constants.ServiceSockDir + "=" + dirs.SockRoot,
 		constants.HomeEnvVarName + "=" + dirs.HomeDir,
+		"NO_COLOR=true",


Can we either constantize this option or add a comment on what it does? I think I know what it means, but I'd like a comment somewhere to confirm.

It's this way because it's an existing standard:

https://www.gnu.org/software/gettext/libtextstyle/manual/html_node/The-NO_005fCOLOR-variable.html

I was not familiar with said standard. Would you add a brief comment please?

test/integration/analytics_int_test.go

test/integration/branch_int_test.go

test/integration/e2eissues_int_test.go

mitchell-as · 2023-09-20T20:56:57Z

test/integration/install_scripts_int_test.go

-	cp.ExpectLongString("Installation path must be an empty directory")
-	cp.Wait()
+	cp.Expect("Installation path must be an empty directory")
+	cp.ExpectExit()


How about expecting a non-zero exit code? This looks like an error.

I should've added a comment, cause I don't remember the specifics. But looking at the commit and the context I believe I did this because the exit code doesn't bubble up properly through powershell.

I think it's better to have some comment here rather than none at all. If I saw this down the road I would be very tempted to change it to ExpectExitCode(0).

test/integration/vscode_int_test.go

Naatan · 2023-09-25T19:05:15Z

This was a herculean effort that you managed to pull off. No amount of applause is enough. In addition to the overhaul, I like that you also added reasonable timeouts or removed unnecessary ones.

Thank you! Worth noting that I still want to work more on the timeouts, just not in this PR.

I'd like to end up with a default timeout of 5 seconds, and have anything that needs more specifically state it. But as you might imagine that could itself take days to untangle.

mitchell-as · 2023-09-25T19:19:04Z

internal/testhelpers/e2e/session.go

+	if spawnOpts.RunInsideShell {
+		switch runtime.GOOS {
+		case "windows":
+			shell = "cmd.exe"


I see you added the Cmd constant on line 295, but you didn't change the one here. Unless I'm missing something, best to be safe and change it here too, right?

mitchell-as · 2023-09-25T19:20:54Z

test/integration/install_scripts_int_test.go

-	cp.ExpectLongString("Installation path must be an empty directory")
-	cp.Wait()
+	cp.Expect("Installation path must be an empty directory")
+	cp.ExpectExit()


I think it's better to have some comment here rather than none at all. If I saw this down the road I would be very tempted to change it to ExpectExitCode(0).

mitchell-as

Almost there!

Naatan added 4 commits June 28, 2023 09:44

Replacing termtest v1 with v2

1307185

WIP: Move tests to termtest v2

be9a8be

Update termtest package to get terminal emulation fix

de70855

Add missing import

267cc04

Naatan added the Test: critical label Aug 23, 2023

github-actions bot changed the base branch from master to version/0-41-0-RC1 August 23, 2023 21:01

Naatan added 7 commits August 23, 2023 14:40

@Naatan

b035e17

Fix tests to be termtest compatible

Fix activate test

7a4740d

Use Output() instead of Snapshot() as that was what the original imp…

22b87e8

…lementation did, despite the name

Update modules

62a713e

Fix tests using Send() where they should be using SendLine()

ef1f227

Update termtest to get race condition fix with Send()

e4f10ad

Naatan force-pushed the DX-1910 branch from 3a069d5 to e4f10ad Compare August 23, 2023 22:14

Naatan added 10 commits August 24, 2023 11:54

Update termtest

68a4734

Don't continue running test when they can't possibly succeed

4d54662

Fix ExpectInput not matching the right thing

22d365c

Don't rely on SHELL

bef1afc

Fix SpawnShell failing if there are no further args

b67f686

Don't run shells within shells

d2b2e30

Update termtest

c4fc3ed

Leave default to run inside shell for now

29caa61

Update termtest

dbc69da

Ensure errors are surfaced

ff93152

Naatan added the Test: all label Aug 28, 2023

Naatan added 5 commits August 28, 2023 11:45

Update termtest to get windows fixes

1f57ac0

Update termtest

db8a297

Fix interrupt test

728c3ad

Work around line ending issue

0129cb0

Fix test depending on terminal col width

3d8ed79

Naatan added 7 commits September 19, 2023 12:47

Update tests to use new API

69e8445

Merge branch 'version/0-42-0-RC1' into DX-1910

f40a517

# Conflicts: # go.sum # test/integration/install_scripts_int_test.go # test/integration/update_int_test.go

Update tests to termtest v2 api

4da3f5b

Fix test

f5f92c0

Fix test

fb4684e

Give windows more time to delete files

205678b

Merge branch 'version/0-42-0-RC1' into DX-1910

5ec5c03

# Conflicts: # internal/testhelpers/e2e/session.go

Naatan requested a review from mitchell-as September 20, 2023 18:58

mitchell-as requested changes Sep 20, 2023

View reviewed changes

Naatan added 8 commits September 25, 2023 11:44

Fix typo

65cd5d8

Fix incorrect name in comment

03a4736

Make comment more specific

a1bf3d0

Add link to jira ticket

b93bacc

Add comments explaining flags

dd8cb15

Use variable instead of hard coded string

5ed621c

Add comment

89d6a7a

Add comment with rationale to ExpectRE

5331492

Naatan requested a review from mitchell-as September 25, 2023 19:05

mitchell-as reviewed Sep 25, 2023

View reviewed changes

mitchell-as requested changes Sep 25, 2023

View reviewed changes

Naatan added 4 commits September 25, 2023 12:42

Merge remote-tracking branch 'origin/version/0-42-0-RC1' into DX-1910

c1debf8

Update new tests to use termtest v2 api

68fa6b9

Use cmd var

fea0746

Add comment with rationale about exit code

3d87ecb

Naatan requested a review from mitchell-as September 25, 2023 19:45

mitchell-as approved these changes Sep 25, 2023

View reviewed changes

Naatan merged commit 99bbe2f into version/0-42-0-RC1 Sep 25, 2023
3 of 6 checks passed

Naatan deleted the DX-1910 branch September 25, 2023 21:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update to termtest v2 #2731

Update to termtest v2 #2731

Naatan commented Aug 23, 2023

Naatan commented Sep 20, 2023

mitchell-as left a comment

mitchell-as Sep 20, 2023

Naatan Sep 25, 2023

mitchell-as Sep 25, 2023

mitchell-as Sep 20, 2023

Naatan Sep 25, 2023

mitchell-as Sep 25, 2023

mitchell-as Sep 20, 2023

Naatan Sep 25, 2023

mitchell-as Sep 25, 2023

Naatan commented Sep 25, 2023

mitchell-as Sep 25, 2023

mitchell-as Sep 25, 2023

mitchell-as left a comment

Update to termtest v2 #2731

Update to termtest v2 #2731

Conversation

Naatan commented Aug 23, 2023

Naatan commented Sep 20, 2023

mitchell-as left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Naatan commented Sep 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mitchell-as left a comment

Choose a reason for hiding this comment